Use global success value in acknowledgment #7621
Conversation
| } else { | ||
| if res.Status == types.PacketStatus_Failure { | ||
| // Set RecvSuccess to false | ||
| ack.RecvSuccess = false | ||
| // Modify events in cached context to reflect unsuccessful acknowledgement | ||
| sdkCtx.EventManager().EmitEvents(internalerrors.ConvertToErrorEvents(cacheCtx.EventManager().Events())) |
There was a problem hiding this comment.
We can break here and just have no AppAcknowledgements on an error.
There was a problem hiding this comment.
I looked into doing this. It became too complicated because it caused empty acks to be valid which I didn't want to do. Otherwise I need to create a sentinel ack and fill it in for the acks that didn't error which was more complicated than necessary especially since we only support single payload for now
| } else { | ||
| if res.Status == types.PacketStatus_Failure { | ||
| // Set RecvSuccess to false | ||
| ack.RecvSuccess = false | ||
| // Modify events in cached context to reflect unsuccessful acknowledgement | ||
| sdkCtx.EventManager().EmitEvents(internalerrors.ConvertToErrorEvents(cacheCtx.EventManager().Events())) |
There was a problem hiding this comment.
I looked into doing this. It became too complicated because it caused empty acks to be valid which I didn't want to do. Otherwise I need to create a sentinel ack and fill it in for the acks that didn't error which was more complicated than necessary especially since we only support single payload for now
| @@ -212,7 +217,7 @@ func (k *Keeper) Acknowledgement(ctx context.Context, msg *types.MsgAcknowledgem | |||
| for i, pd := range msg.Packet.Payloads { | |||
| cbs := k.Router.Route(pd.SourcePort) | |||
| ack := msg.Acknowledgement.AppAcknowledgements[i] | |||
| err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, ack, pd, relayer) | |||
| err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, msg.Acknowledgement.RecvSuccess, ack, pd, relayer) | |||
There was a problem hiding this comment.
Note: We will need to tweak this eventually since it would be weird to call ack callback with a successful ack bytes but a false recvSuccess but not necessary for single payload change
There was a problem hiding this comment.
Might be good to add a TODO comment here for that
AdityaSripal
requested review from
damiannolan,
DimitrisJim,
gjermundgaraba and
bznein
as code owners
AdityaSripal
changed the title
AdityaSripal
requested review from
damiannolan and
srdtrk
and removed request for
damiannolan
|
| @@ -212,7 +217,7 @@ func (k *Keeper) Acknowledgement(ctx context.Context, msg *types.MsgAcknowledgem | |||
| for i, pd := range msg.Packet.Payloads { | |||
| cbs := k.Router.Route(pd.SourcePort) | |||
| ack := msg.Acknowledgement.AppAcknowledgements[i] | |||
| err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, ack, pd, relayer) | |||
| err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, msg.Acknowledgement.RecvSuccess, ack, pd, relayer) | |||
There was a problem hiding this comment.
Might be good to add a TODO comment here for that
Description
closes: #7645
closes: #7472
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.